Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stricter handling of input #5

Merged
merged 5 commits into from May 6, 2013
Merged

Stricter handling of input #5

merged 5 commits into from May 6, 2013

Conversation

vung
Copy link
Contributor

@vung vung commented Feb 5, 2012

This is a set of changes to the way input is handled in Peppercorn.

In short:

  1. Use a single, module-specific, exception type for parsing errors and document that parse() might raise an exception.
  2. Eliminate use of recursion and either parse successfuly the whole input or raise an exception

The recommended usage doesn't mention that parse() might raise exceptions.
The three places [1] I saw using parse() assume it returns sucessfully, so it's easy to feed input to a form that ultimately results in a server error.
I don't know if people really expected it to not raise an exception or simply followed the documented example.

The test suite already accounts for ValueError. Aditionally, peppercorn.parse() might raise:

  1. RuntimeError, when the input (even a well-formed one) is nested deeper than the recursion limit
  2. StopIteration when a sequence is started and not closed.

Presumably the correct usage would be:

try:
    pstruct = peppercorn.parse(fields)
except (RuntimeError, ValueError, StopIteration):
    # malformed syntax
    raise HTTPBadRequest()

It assumes access to a framework-specific exception, though.

The first commit establishes a ParseError, which hopefully makes it easier to target exceptions raised by peppercorn.parse().

The second commit takes the documentation at face value ("Mappings and sequences can be nested arbitrarily") and makes it so. This is done by replacing the recursive implementation with one that uses a list as a stack.

Doing this brought on another thing: peppercorn.parse() handles input that contains extra end markers by simply returning what was collected up to that point.

The non-recursive implementation passed the full test suite even if (as I discoverd later) it handled extra end markers differently.

I added some tests and contorted it a bit to preserve the original behaviour.

I still think that an input like [('foo', 'fred'), ('__end__', ''), ('bar', 'joe')] should raise an exception and not be parsed as {'foo': 'fred'}.

This is done in the last commit. I separated the end tests as they might be useful even if you disagree.

[1]
https://github.com/Pylons/deform/blob/master/deform/field.py#L502
https://bitbucket.org/danjac/pyramid_simpleform/src/32d81533f51b/pyramid_simpleform/form.py#cl-107
https://bitbucket.org/ianjosephwilson/pepperedform/src/10b60a55c737/pepperedform/handler.py#cl-16

@mcdonc
Copy link
Member

mcdonc commented Feb 5, 2012

These changes look good. Would you be willing to "sign" the CONTRIBUTORS.txt in your fork?

@tseaver tseaver merged commit 570f351 into Pylons:master May 6, 2013
@tseaver
Copy link
Member

tseaver commented May 6, 2013

Thanks for the patch! I merged only the contributor signature, since #7 obsoletes the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants